Skip to content

Swift: Improvements related to the swift/cleartext-logging query.#13980

Merged
geoffw0 merged 15 commits intogithub:mainfrom
geoffw0:logfix
Sep 18, 2023
Merged

Swift: Improvements related to the swift/cleartext-logging query.#13980
geoffw0 merged 15 commits intogithub:mainfrom
geoffw0:logfix

Conversation

@geoffw0
Copy link
Copy Markdown
Contributor

@geoffw0 geoffw0 commented Aug 16, 2023

Improvements related to the swift/cleartext-logging query:

  • model flow through getVaList (so that we can pass the NSLogv test cases).
  • add data flow through variadic arguments (this will have effects more widely than the swift/cleartext-logging query and is separately tested).
  • add an allowImplicitRead for array content sinks in the swift/cleartext-logging query (so that, together with the previous bullet point, we can pass the NSLog and more of the print test cases).
  • add more sinks for the swift/cleartext-logging query.

@geoffw0 geoffw0 added the Swift label Aug 16, 2023
@geoffw0 geoffw0 requested a review from a team as a code owner August 16, 2023 09:59
@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Sep 11, 2023

Fixed merge conflicts.

@MathiasVP possibly VarargExpansionExpr is yet another example of something that should be hidden from the AST instead of having a special case in data flow? As far as I can tell where varargs are used a VarargExpansionExpr wraps the ArrayExpr entering the call.

@geoffw0 geoffw0 mentioned this pull request Sep 11, 2023
Merged
@MathiasVP
Copy link
Copy Markdown
Contributor

MathiasVP commented Sep 11, 2023

Fixed merge conflicts.

@MathiasVP possibly VarargExpansionExpr is yet another example of something that should be hidden from the AST instead of having a special case in data flow? As far as I can tell where varargs are used a VarargExpansionExpr wraps the ArrayExpr entering the call.

Good point. Looking at https://github.com/apple/swift/blob/main/include/swift/AST/Expr.h#L3566 it looks like there are plans to have surface-level syntax for VarargExpansionExprs. Once we have surface-syntax for it I don't think we want to hide it from the AST?

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Sep 11, 2023

Looking at https://github.com/apple/swift/blob/main/include/swift/AST/Expr.h#L3566 it looks like there are plans to have surface-level syntax for VarargExpansionExprs.

Sounds like I should probably leave it as a data flow solution then, at least, for now.

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Sep 13, 2023

DCA LGTM.

Merge conflicts fixed (again).

@geoffw0
Copy link
Copy Markdown
Contributor Author

geoffw0 commented Sep 18, 2023

Fixed merge conflicts again.

MathiasVP
MathiasVP previously approved these changes Sep 18, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@geoffw0 geoffw0 merged commit 4323bee into github:main Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MathiasVP MathiasVP MathiasVP approved these changes

Assignees

No one assigned

Labels

documentation Swift

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@geoffw0 @MathiasVP